Skip to content

refactor: install clang tools on demand#279

Open
2bndy5 wants to merge 25 commits intomainfrom
clang-tools-downloader
Open

refactor: install clang tools on demand#279
2bndy5 wants to merge 25 commits intomainfrom
clang-tools-downloader

Conversation

@2bndy5
Copy link
Copy Markdown
Collaborator

@2bndy5 2bndy5 commented Mar 10, 2026

In order of precedence, this patch allows installing clang tools from

  1. PyPI
    • Python and pip are not required.
      The executable binary is extracted from the downloaded wheel.
    • downloaded wheels are verified using SHA256 or Blake2b256 algorithms.
  2. system package manager(s) (if any available are supported)
    • Windows: chocolatey, winget
    • MacOS: homebrew
    • Linux: apt (Debian), dnf (Fedora), pacman (arch linux)
      • Note: For apt, when first attempt fails, the official LLVM install script is (cached and) used as a fallback approach.
  3. static binaries
    • binaries are verified with accompanied SHA512 checksum
    • min and max supported versions can be controlled via env variables at compile time (not runtime); MIN_CLANG_TOOLS_VERSION and MAX_CLANG_TOOLS_VERSION respectively.
    • static binary release tag can be controlled via env variable CLANG_TOOLS_TAG at compile time (not runtime).
    • static binary repo url can be controlled via env variable CLANG_TOOLS_REPO at compile time (not runtime).

Caching

Caching is enabled per OS standards and specific to cpp-linter use.
Caching does not apply to package managers as they tend to manage caches on their own.
Note, the cached official LLVM install script (for the apt package manager fallback) becomes invalid 24 hours after download.
Cache directory can be overridden using the CPP_LINTER_CACHE environment variable.


Also upgrade cargo/rust dependencies.

Summary by CodeRabbit

  • New Features
    • Adds a clang-installer CLI to fetch/install clang-format/clang-tidy (native package managers, PyPI, or static binaries) with caching, integrity checks, progress display, and warmup runs.
  • Chores
    • Centralized workspace dependency management and added an opt-in binary feature to gate CLI builds.
    • CI/test workflows improved: warmup profile, looped clang-tool coverage across versions, and adjusted build matrix/feature flags.
  • Compatibility
    • Node binding main is now synchronous; TypeScript declaration updated to non-Promise return.

@2bndy5 2bndy5 added enhancement New feature or request dependencies Pull requests that update a dependency file rust Pull requests that update Rust code labels Mar 10, 2026
@2bndy5 2bndy5 linked an issue Mar 10, 2026 that may be closed by this pull request
@2bndy5 2bndy5 force-pushed the clang-tools-downloader branch 12 times, most recently from 89de56d to f005df3 Compare March 12, 2026 12:45
@2bndy5

This comment was marked as resolved.

2bndy5 added 6 commits March 29, 2026 13:55
In order of precedence, this patch allows installing clang tools from
1. PyPI (downloaded wheels are verified using SHA256 or Bake2b256)
2. system package manager(s) (if available)
3. static binaries (verified with accompanied SHA512 checksum)

Also upgrade cargo/rust dependencies.
disables any API that is specific to building a binary. Thus CLI-specific API is not exposed in the library API.

Also reviewed docs
also check cache before (re)populating it
@2bndy5 2bndy5 force-pushed the clang-tools-downloader branch from f005df3 to 6b5a77b Compare March 29, 2026 21:01
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 29, 2026

Codecov Report

❌ Patch coverage is 82.94340% with 226 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.71%. Comparing base (a7f94cb) to head (03225d5).
⚠️ Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
...g-installer/src/downloader/native_packages/unix.rs 24.02% 117 Missing ⚠️
clang-installer/src/version.rs 86.04% 24 Missing ⚠️
...nstaller/src/downloader/native_packages/windows.rs 81.48% 20 Missing ⚠️
clang-installer/src/progress_bar.rs 80.00% 16 Missing ⚠️
...ng-installer/src/downloader/native_packages/mod.rs 70.83% 14 Missing ⚠️
clang-installer/src/downloader/pypi.rs 95.56% 13 Missing ⚠️
clang-installer/src/downloader/static_dist.rs 90.42% 9 Missing ⚠️
clang-installer/src/downloader/mod.rs 93.54% 6 Missing ⚠️
clang-installer/src/tool.rs 97.47% 3 Missing ⚠️
clang-installer/src/utils.rs 95.74% 2 Missing ⚠️
... and 1 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #279      +/-   ##
==========================================
- Coverage   96.90%   92.71%   -4.20%     
==========================================
  Files          14       26      +12     
  Lines        3167     4295    +1128     
==========================================
+ Hits         3069     3982     +913     
- Misses         98      313     +215     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@2bndy5 2bndy5 force-pushed the clang-tools-downloader branch from 3c9fa8a to 068e76c Compare March 30, 2026 03:30
@2bndy5 2bndy5 force-pushed the clang-tools-downloader branch from 068e76c to a84fa70 Compare March 30, 2026 05:13
@2bndy5 2bndy5 marked this pull request as ready for review March 30, 2026 05:49
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 30, 2026

Walkthrough

Adds a new workspace crate clang-installer (downloaders, verifiers, native installers, CLI/binary), centralizes workspace dependency versions, gates CLI/binary behavior behind a bin feature, migrates clang-version handling to semver::Version, and updates CI to run a warmup profile with a nushell-driven clang-version loop.

Changes

Cohort / File(s) Summary
New clang-installer crate
clang-installer/Cargo.toml, clang-installer/README.md, clang-installer/src/lib.rs, clang-installer/src/main.rs, clang-installer/src/progress_bar.rs, clang-installer/src/utils.rs, clang-installer/src/tool.rs, clang-installer/src/version.rs, clang-installer/src/downloader/..., clang-installer/tests/*
Adds a complete crate: cache & hashing, PyPI/static-dist downloaders, native package-manager installers (unix/windows), tool/version resolution and symlinking, progress bar, CLI binary (clang-tools) gated by bin, and unit/integration tests.
Workspace & manifests
Cargo.toml, cpp-linter/Cargo.toml, bindings/node/Cargo.toml, bindings/python/Cargo.toml, docs/Cargo.toml
Adds clang-installer to workspace members, introduces [workspace.dependencies], converts many crates to workspace-managed deps, adds a bin feature and requires it for binaries, and wires cpp-linterclang-installer path dependency.
Downloader/core & verification
clang-installer/src/downloader/mod.rs, clang-installer/src/downloader/caching.rs, clang-installer/src/downloader/hashing.rs, clang-installer/src/downloader/pypi.rs, clang-installer/src/downloader/static_dist.rs, clang-installer/src/downloader/native_packages/*
Implements download helper, cache directory logic, file-hash verification (sha256/blake2b/sha512), PyPI wheel discovery/extract, static-distribution selection & sha512 checking, and native package-manager abstraction plus unix/windows installers.
CLI binary & runtime
clang-installer/src/main.rs, clang-installer/Cargo.toml, cpp-linter/Cargo.toml
Adds clang-tools binary entry (gated by bin), CLI parsing, logging initialization, and integrates binary feature gating into workspace crates.
cpp-linter integration & refactor
cpp-linter/src/clang_tools/mod.rs, cpp-linter/src/clang_tools/clang_tidy.rs, cpp-linter/src/cli/..., cpp-linter/src/run.rs, cpp-linter/src/common_fs/*, cpp-linter/src/logger.rs, cpp-linter/src/rest_api/*
Removes local ClangTool/RequestedVersion implementations in favor of clang_installer::{ClangTool, RequestedVersion}, changes ClangVersions fields to Option<semver::Version>, gates many CLI parsing types behind feature = "bin", and updates call sites/tests accordingly.
Bindings (Node/Python) & types
bindings/node/src/lib.rs, bindings/node/Cargo.toml, bindings/node/index.d.ts, bindings/python/Cargo.toml
Node binding main converted from async → sync by constructing a Tokio runtime; Cargo features updated to enable bin; TypeScript declaration changed to return void. Python binding Cargo uses workspace-managed deps and bin enabled on cpp-linter path dep.
CI, tests & tooling
.config/nextest.toml, .github/workflows/run-dev-tests.yml, .github/workflows/benchmark.yml, .github/workflows/binary-builds.yml, .github/workflows/benchmark.yml, nurfile, .pre-commit-config.yaml, cspell.config.yml
Nextest: exclude #*eval_version from default/ci/all and add warmup profile; CI: replace repeated clang installs with a nushell loop running warmup then ci/all; workflows adjusted to pass --features bin and tweak matrices; minor pre-commit/cspell updates.
Tests, docs, miscellaneous
clang-installer/README.md, clang-installer/tests/*, cpp-linter/README.md, cpp-linter/tests/*, cpp-linter/tests/demo/.clang-tidy, benchmark/benchmark.nu
Adds installer README and integration tests (PyPI/static), test logging helpers, removes a README paragraph, tweaks a clang-tidy setting, and reads benchmark files as raw content.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related issues

Possibly related PRs

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'refactor: install clang tools on demand' accurately summarizes the main change—introducing on-demand installation of clang tools instead of assuming they are pre-installed or always available.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch clang-tools-downloader

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 20

Note

Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
cpp-linter/src/rest_api/mod.rs (1)

146-165: ⚠️ Potential issue | 🔴 Critical

Bug: Version fields are swapped between format and tidy comments.

The code passes the wrong version to each comment function:

  • Line 152: make_format_comment receives tidy_version but should receive format_version
  • Line 162: make_tidy_comment receives format_version but should receive tidy_version

The inline comments also have it backwards, confirming this is a copy-paste error. This will cause the generated PR comments to display incorrect tool versions.

🐛 Proposed fix
             if format_checks_failed > 0 {
                 make_format_comment(
                     files,
                     &mut comment,
                     format_checks_failed,
-                    // tidy_version should be `Some()` value at this point.
-                    &clang_versions.tidy_version.as_ref().unwrap().to_string(),
+                    // format_version should be `Some()` value at this point.
+                    &clang_versions.format_version.as_ref().unwrap().to_string(),
                     &mut remaining_length,
                 );
             }
             if tidy_checks_failed > 0 {
                 make_tidy_comment(
                     files,
                     &mut comment,
                     tidy_checks_failed,
-                    // format_version should be `Some()` value at this point.
-                    &clang_versions.format_version.as_ref().unwrap().to_string(),
+                    // tidy_version should be `Some()` value at this point.
+                    &clang_versions.tidy_version.as_ref().unwrap().to_string(),
                     &mut remaining_length,
                 );
             }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@cpp-linter/src/rest_api/mod.rs` around lines 146 - 165, The
make_format_comment and make_tidy_comment calls are passing the swapped version
fields; change the arguments so make_format_comment receives
clang_versions.format_version.as_ref().unwrap().to_string() and
make_tidy_comment receives
clang_versions.tidy_version.as_ref().unwrap().to_string(), keeping the other
parameters (files, &mut comment, *_checks_failed, &mut remaining_length)
unchanged so the correct tool versions are shown.
cpp-linter/src/cli/structs.rs (1)

246-289: ⚠️ Potential issue | 🔴 Critical

Gate the remaining Clap-based tests behind bin.

display_lines_changed_only_enum() and display_thread_comments_enum() call ValueEnum::from_str(), but those trait implementations only exist under #[cfg(feature = "bin")]. The unconditional Cli import on line 251 has the same problem—Cli is only defined when the feature is enabled. Running cargo test without --features bin will fail to compile.

Suggested fix
 #[cfg(test)]
 mod test {
     #[cfg(feature = "bin")]
     use clap::{Parser, ValueEnum};

-    use super::{Cli, LinesChangedOnly, ThreadComments};
+    #[cfg(feature = "bin")]
+    use super::Cli;
+    use super::{LinesChangedOnly, ThreadComments};

     #[test]
     #[cfg(feature = "bin")]
     fn parse_positional() {
         let cli = Cli::parse_from(["cpp-linter", "file1.c", "file2.h"]);
@@
     }

     #[test]
+    #[cfg(feature = "bin")]
     fn display_lines_changed_only_enum() {
         let input = "Diff";
         assert_eq!(
             LinesChangedOnly::from_str(&input, true).unwrap(),
             LinesChangedOnly::Diff
@@
     }

     #[test]
+    #[cfg(feature = "bin")]
     fn display_thread_comments_enum() {
         let input = "Update";
         assert_eq!(
             ThreadComments::from_str(input, true).unwrap(),
             ThreadComments::Update
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@cpp-linter/src/cli/structs.rs` around lines 246 - 289, The failing compile is
caused by using Clap-only symbols when the "bin" feature is disabled; wrap the
Clap-dependent import and tests with the feature gate: move or annotate use
super::{Cli, LinesChangedOnly, ThreadComments} so that Cli is only imported
under #[cfg(feature = "bin")] and add #[cfg(feature = "bin")] to the
parse_positional, display_lines_changed_only_enum, and
display_thread_comments_enum tests (or alternatively gate the entire test module
with #[cfg(feature = "bin")]) so that LinesChangedOnly::from_str and
ThreadComments::from_str are only referenced when the "bin" feature is enabled.
🟡 Minor comments (2)
cpp-linter/src/logger.rs-2-2 (1)

2-2: ⚠️ Potential issue | 🟡 Minor

Add feature gate to logger module export in lib.rs.

The logger module is gated with #![cfg(feature = "bin")] in its own file, but line 14 of lib.rs exports it unconditionally with pub mod logger;. This will cause compilation failure when the bin feature is disabled. The export must be gated: #![cfg(feature = "bin")] pub mod logger;.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@cpp-linter/src/logger.rs` at line 2, The lib export for the logger module is
unconditional while the module file itself is gated with #![cfg(feature =
"bin")]; update the module export so it is also feature-gated by changing the
`pub mod logger;` export to be annotated with the same cfg (i.e., add
`#[cfg(feature = "bin")]` before the `pub mod logger;`) so the compiler only
exports the logger when the `bin` feature is enabled.
.github/workflows/run-dev-tests.yml-107-117 (1)

107-117: ⚠️ Potential issue | 🟡 Minor

Prevent redundant eval_version test execution.

The warmup profile runs eval_version tests, but the ci profile (used for all non-max versions) also includes them via its default-filter:

default-filter = "kind(test) + test(#*use_extra_args) + test(#*eval_version)"

This causes eval_version tests to run twice per clang version (except the max version). Update the ci profile filter to exclude eval_version tests:

default-filter = "kind(test) + test(#*use_extra_args) - test(#*eval_version)"

This aligns with the all profile which intentionally excludes them.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/run-dev-tests.yml around lines 107 - 117, The CI is
running eval_version tests twice because the "ci" profile's default-filter still
includes eval_version; update the "ci" profile's default-filter (the
default-filter setting for the ci profile) to exclude eval_version tests by
replacing the inclusion of test(#*eval_version) with an exclusion (-
test(#*eval_version)), matching how the "all" profile intentionally excludes
them so warmup + ci do not duplicate eval_version runs.
🧹 Nitpick comments (7)
clang-installer/README.md (1)

5-7: Tighten wording for API scope clarity.

This is accurate, but the sentence reads a bit awkwardly. Consider: “This utility is used by the cpp-linter CLI, and its API is internal to the cpp-linter crate.”

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@clang-installer/README.md` around lines 5 - 7, Replace the awkward sentence
"This is a utility for cpp-linter CLI, thus its API is considered internal to
cpp-linter crate." with a tighter phrasing such as: "This utility is used by the
cpp-linter CLI, and its API is internal to the `cpp-linter` crate." Update the
README.md sentence to match that suggested wording for clarity and grammar.
clang-installer/src/tool.rs (1)

185-191: Symlink validation only confirms readability, not target validity.

fs::read_link() verifies the symlink itself is readable but doesn't confirm the target file exists or is executable. If the target was deleted, this would still succeed but the symlink would be broken.

Consider adding symlink.exists() check or using fs::metadata(symlink_path) to verify the final target is accessible.

💡 Optional enhancement to validate symlink target
         // check if the symlink is valid and points to an existing file.
         let symlink = fs::read_link(symlink_path)?;
+        if !symlink_path.exists() {
+            return Err(std::io::Error::new(
+                std::io::ErrorKind::NotFound,
+                format!("Symlink target does not exist: {}", symlink.to_string_lossy()),
+            ));
+        }
         log::info!(
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@clang-installer/src/tool.rs` around lines 185 - 191, The current code uses
fs::read_link(symlink_path) and logs the link but doesn't verify the link's
target exists or is accessible; update the logic around symlink_path and the
symlink variable to resolve the final target (e.g., join symlink_path.parent()
with symlink) and then call fs::metadata or Path::exists on that resolved target
to ensure the file exists (and optionally is executable) before proceeding; keep
the existing log but add an error path that handles broken symlinks when
fs::metadata(target) fails.
clang-installer/Cargo.toml (1)

21-38: Consider documenting why crypto crates are pinned locally.

blake2 (0.10.6) and sha2 (0.10.9) are pinned directly rather than via workspace.dependencies. This is reasonable for cryptographic stability, but a brief comment explaining the rationale would help future maintainers.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@clang-installer/Cargo.toml` around lines 21 - 38, Add an inline comment in
Cargo.toml next to the blake2 = "0.10.6" and sha2 = "0.10.9" dependency entries
explaining that these crypto crates are intentionally pinned locally (not in
workspace.dependencies) for cryptographic stability/security and to avoid
accidental upgrades that could change hashing outputs or break binary
compatibility; reference the blake2 and sha2 lines so maintainers understand the
deliberate pinning and know not to move them to workspace.dependencies without
review.
clang-installer/src/downloader/caching.rs (1)

29-43: Consider: StaticDistDownloader bypasses is_cache_valid.

Based on clang-installer/src/downloader/static_dist.rs (lines 145, 159), StaticDistDownloader directly checks .exists() instead of using is_cache_valid. While this is functionally equivalent when max_age = None, it creates an inconsistent pattern. If cache eviction policies change in the future, StaticDistDownloader won't benefit.

This is optional since the current behavior is correct—just noting for future maintainability.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@clang-installer/src/downloader/caching.rs` around lines 29 - 43,
StaticDistDownloader currently bypasses the shared cache validation logic by
directly calling .exists() on the cache file; update StaticDistDownloader (the
code in StaticDistDownloader::... where it checks cache_file.exists()) to call
the shared helper is_cache_valid(cache_file, max_age) instead so it honors the
same max_age/eviction semantics as is_cache_valid; ensure you pass the same
max_age used elsewhere and remove the direct .exists() check so future cache
policy changes apply consistently.
.github/workflows/run-dev-tests.yml (1)

34-34: Minor: Inconsistent quoting in matrix.

"windows-latest" is quoted while ubuntu-latest is not. While YAML handles both, consistent quoting improves readability.

-        os: ["windows-latest", ubuntu-latest]
+        os: [windows-latest, ubuntu-latest]
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/run-dev-tests.yml at line 34, The matrix OS entries under
the "os" key are inconsistently quoted; update the matrix so both entries use
the same quoting style (either both quoted or both unquoted) for consistency and
readability—locate the "os" matrix line in the workflow (os: ["windows-latest",
ubuntu-latest]) and make the two entries uniform, e.g., change ubuntu-latest to
"ubuntu-latest" to match "windows-latest".
clang-installer/tests/pypi.rs (1)

45-48: Consider extracting assertions into the test function.

The setup() function performs both setup and assertions, making it harder to understand test structure. The test function download_clang_format just calls setup().await with no additional assertions.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@clang-installer/tests/pypi.rs` around lines 45 - 48, The test currently hides
assertions inside setup(), so update the code so setup() only performs
initialization and move the assertions into the test function
download_clang_format; locate the async fn setup() and remove any
assert!/assert_eq!/expect calls from it, then call setup().await inside
download_clang_format and add the relevant assertions there (or split setup into
setup() and verify_* helpers if preferred) so the test function explicitly
contains the verification logic.
clang-installer/src/main.rs (1)

120-122: Validate --directory in the CLI layer.

Passing an existing file path here will currently fail later in the downloader with a generic IO error. A custom clap value parser that accepts missing paths and existing directories, but rejects existing files, would keep this concern in the CLI.

Based on learnings: In the cpp-linter-rs project, path validation (such as checking whether a path is a file or directory) should be performed in CLI parsing (cpp-linter/src/cli/structs.rs) rather than in the tool lookup logic (cpp-linter/src/clang_tools/mod.rs). This maintains proper separation of concerns.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@clang-installer/src/main.rs` around lines 120 - 122, Add a clap value parser
for the `directory` argument that accepts non-existent paths and existing
directories but rejects existing files; implement a function like
`validate_install_dir` (signature returning Result<PathBuf, String> or
implementing TypedValueParser) which converts the input to a PathBuf, returns Ok
for paths that don't exist or that are directories, and returns Err with a clear
message when the path exists and is a file, then attach it to the `pub
directory: Option<PathBuf>` arg via the `#[arg(value_parser =
validate_install_dir)]` attribute so validation happens in the CLI layer instead
of later in downloader/tool lookup code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@bindings/node/src/lib.rs`:
- Around line 6-13: The exported N-API function main was made synchronous (pub
fn main) but the TypeScript declaration and tests still expect a Promise and a
numeric return, and the tokio runtime build uses .unwrap() which can panic; fix
by either reverting main to an async function (pub async fn main) so napi-rs
will return a Promise and preserve the existing index.d.ts/test expectations, or
if you intend main to be synchronous keep pub fn main but regenerate
bindings/node/index.d.ts to remove the Promise and adjust the test to expect the
synchronous void/number contract; in both cases replace the .unwrap() on
tokio::runtime::Builder::new_multi_thread().build() with proper error handling
(map the build error into Error::new(Status::GenericFailure, ... ) or propagate
it) and ensure run_main(args) is invoked consistently (await when async,
block_on when keeping sync) so the Rust signature, JS declaration, and tests are
aligned.

In `@bindings/python/Cargo.toml`:
- Line 20: The pyo3 dependency declaration currently sets features =
["abi3-py39"] which overrides workspace defaults and drops the required
extension-module feature; update the pyo3 entry in bindings/python/Cargo.toml so
its features include both "abi3-py39" and "extension-module" (i.e., add
"extension-module" alongside the existing "abi3-py39") to ensure the
extension-module workspace feature is not lost.

In `@Cargo.toml`:
- Line 31: The workspace currently builds reqwest without any TLS backend but
publicly exports the rest_api module which requires reqwest::Client for HTTPS;
fix this by adding an explicit library TLS feature and gating rest_api behind
it: add a new feature (e.g., "tls") in Cargo.toml that enables reqwest with
native-tls (adjust the reqwest dependency entry to include feature =
["native-tls", "http2", "charset"] under that feature), remove implicit reliance
on the existing "bin" feature for TLS, and wrap the public export of the
rest_api module (e.g., the pub mod rest_api declaration in lib.rs) with
#[cfg(feature = "tls")] so consumers must opt in to TLS support; alternatively,
if you prefer TLS always available, make the library default feature include
"tls" instead of tying it to "bin".

In `@clang-installer/src/downloader/hashing.rs`:
- Around line 37-42: The read loops currently use "while let Ok(bytes_read) =
file_reader.read(&mut buf)" which swallows I/O errors and leads to partial-hash
finalization; change each loop (the three branches using file_reader.read +
hasher.update + hasher.finalize) to explicitly propagate read errors instead of
silently breaking — e.g. replace the while-let with a loop that calls
file_reader.read(&mut buf) and uses the ? operator (or maps Err to
DownloadError::IoError) to return on error, breaking only on bytes_read == 0,
then call hasher.update for the bytes read; apply this change to all three
branches that compute the hash so real I/O errors are returned rather than
producing HashMismatch.

In `@clang-installer/src/downloader/mod.rs`:
- Around line 42-49: The code currently opens and truncates cache_path
(cache_file) before the download is verified, risking partial/corrupt cache
entries; change the logic to create and write to a temporary file in the same
directory (using cache_parent and a unique temp name, e.g.,
cache_path.with_extension(".partial") or using a random suffix), write the
downloaded body into that temp file (use OpenOptions as before), flush/fsync the
temp file and its parent directory, then atomically rename (std::fs::rename) the
temp file to cache_path only on successful completion; ensure error paths remove
the temp file and do not touch cache_path. Use the existing cache_parent,
cache_path, and cache_file identifiers to locate the change.

In `@clang-installer/src/downloader/native_packages/mod.rs`:
- Around line 90-97: The code currently trusts
mgr.is_installed_package(&pkg_name, Some(min_version)) and returns the found
ClangVersion immediately; instead, after calling tool.get_exe_path(...) and
tool.capture_version(...) you must validate that the captured version satisfies
the original RequestedVersion/version_req (not just min_version) before
returning. In practice, in the block containing is_installed_package, call
capture_version, check the captured version against version_req (or the
exact/upper-bound requirement represented by RequestedVersion::Requirement), and
only construct and return ClangVersion when that check passes; if it does not,
log a mismatch (including tool, mgr, version and version_req) and continue so
the installer can try other resolution paths.

In `@clang-installer/src/downloader/native_packages/unix.rs`:
- Around line 115-139: The package install logic incorrectly runs Homebrew under
sudo and uses pacman's -y flag; update the UnixPackageManager match so PacMan
uses ["-S", "--noconfirm"] instead of ["-S", "-y"], and change the Command
creation so Homebrew is never prefixed with sudo (i.e., if self ==
UnixPackageManager::Homebrew call Command::new(self.as_str()) regardless of
Self::has_sudo()). Keep the existing has_sudo() behavior for other managers and
ensure package_id and args are passed the same way.
- Around line 176-179: The check for DNF packages in UnixPackageManager::Dnf
uses `dnf list <package>` which returns success for packages available in
repositories (not only installed); update the logic in the
is_installed_package() branch handling UnixPackageManager::Dnf to run `dnf list
installed <package>` (i.e., add the "installed" subcommand) and base the success
decision on that command's exit status so only packages present in the RPMDB are
considered installed.

In `@clang-installer/src/downloader/native_packages/windows.rs`:
- Around line 71-83: The package table parsing uses split(' ') which preserves
empty fields from padded columns and breaks lookups; in the Chocolatey parsing
change the iterator creation (the variable l produced by line.split(' ')) to use
split_whitespace() instead, and in the Winget parsing replace split(' ').skip(1)
with split_whitespace().skip(1) so tokenization ignores padding when matching
package_name and extracting version strings (refer to the iterator variable l
and the Version::parse call to verify correct token positions).

In `@clang-installer/src/downloader/pypi.rs`:
- Around line 423-448: The extracted binary is being cached/placed only by major
version (bin_name uses ver.major) while the wheel cache uses the full ver, which
can cause incorrect reuse; change the extracted path/name so it includes the
full version (use ver or ver.to_string() instead of ver.major) when building
bin_name and extracted_bin before calling Self::extract_bin(clang_tool,
&cached_wheel, &extracted_bin) so each full-version wheel produces a distinct
extracted executable; if you still want a major-version alias (e.g.,
clang-format-17) create or update that separate symlink/alias after extracting
the fully-versioned binary rather than replacing the extracted file.
- Around line 435-438: The code currently skips integrity checks when
info.digests.first() is None; change the logic in the function that handles PyPI
metadata (the block using info.digests.first() and digest.verify(&cached_wheel))
to fail closed: if info.digests.first() returns None, return an error (or
propagate a VerificationMissing error) instead of proceeding, and only call
digest.verify(&cached_wheel) when a digest is present; update any surrounding
error types/context to provide a clear message that no supported digest was
present for the wheel.
- Around line 171-189: The macOS branch in is_compatible_with_system currently
requires exact arch string matches and thus rejects universal2 wheels; update
the macos handling in is_compatible_with_system so that when
std::env::consts::OS == "macos" it treats self.arch == "universal2" as
compatible for both sys_arch "x86_64" and "aarch64" (in addition to the existing
exact matches), i.e., accept universal2 for both Intel and Apple Silicon while
preserving the existing exact checks for "x86_64" and "arm64".

In `@clang-installer/src/progress_bar.rs`:
- Line 1: The progress bar checks terminal status using stdin rather than
stdout, so carriage-return redraws can be written to a redirected stdout; update
the terminal check to use stdout().is_terminal() everywhere it's used (e.g.,
where is_interactive is computed and the checks around lines 51-52) so the
progress bar only enables interactive redraws when stdout is a terminal; adjust
any imports/usages from stdin to stdout accordingly and ensure Write/Result
types still compile.

In `@clang-installer/src/version.rs`:
- Around line 301-304: Save the previous CPP_LINTER_CACHE environment value with
std::env::var_os before calling TempDir::new(), set CPP_LINTER_CACHE to
tmp_cache_dir.path() via std::env::set_var, and ensure you restore the original
value (with std::env::set_var if Some or std::env::remove_var if None) in a
drop/guard placed before tmp_cache_dir is dropped so the process-global state is
restored during test teardown; reference the TempDir::new(), std::env::set_var,
and std::env::var_os/remove_var calls to locate where to add the
save-and-restore guard.
- Around line 331-347: The test function eval_version currently runs by default
and can perform global installs; change it to be gated behind an explicit opt-in
by either marking it with #[ignore] or checking a CI-only env var (e.g.,
CLANG_INSTALL_TEST) at the start of eval_version and early-return when not set.
Locate the async test function named eval_version (and the use of
RequestedVersion::Requirement.eval_tool) and add the guard so the potentially
dangerous installation code only executes when the env flag is present or when
the test is run with --ignored in CI.
- Around line 137-168: After a successful download (from
PyPiDownloader::download_tool or StaticDistDownloader::download_tool) capture
the actual version via tool.capture_version(&bin) and verify it satisfies the
original version_req before creating the symlink and returning ClangVersion; if
the captured version does not meet version_req, do not symlink — either attempt
the next fallback (e.g., continue to other download methods or
try_install_package) or return Err(GetToolError::NotFound). Update the logic
around where bin is obtained and before tool.symlink_bin(&bin, &symlink_path,
overwrite_symlink) to perform this check and handle failures accordingly.

In `@clang-installer/tests/pypi.rs`:
- Around line 13-17: The test is unsafely modifying process-global state via
env::set_var("CPP_LINTER_CACHE", ...) which races when tests run in parallel;
instead, remove the unsafe env::set_var usage in tests/pypi.rs (where
tmp_cache_dir is created in the setup()/test) and either (A) change the
downloader API call in the test to accept tmp_cache_dir.path() explicitly (pass
the cache dir into the function that downloads/pulls from PyPI rather than
relying on CPP_LINTER_CACHE), or (B) mark the affected tests to run serially via
nextest test-group config; reference the tmp_cache_dir variable, the
env::set_var call, and the setup()/downloader invocation to locate the change.

In `@clang-installer/tests/static_dist.rs`:
- Around line 12-16: The tests download_clang_format_17 and
download_clang_format_12_0_1 race by mutating the global env var
CPP_LINTER_CACHE via env::set_var around TempDir::new; fix by applying the same
solution used in pypi.rs — either group these tests under a serial test group
(so they don't run in parallel) or change the API to accept a cache directory
parameter and pass tmp_cache_dir.path() directly instead of setting
CPP_LINTER_CACHE; update the test(s) referencing TempDir::new and remove the
unsafe env::set_var usage, or wrap the tests in a serial test annotation to
ensure isolated execution.

In `@cpp-linter/src/run.rs`:
- Line 6: The feature-gate on run.rs prevents integration tests from importing
run::run_main; remove or change it so tests compile: either remove the
crate-level attribute #![cfg(feature = "bin")] from run.rs (so the module and
function run_main are always compiled), or keep the gate and add
required-features = ["bin"] to the test target in Cargo.toml for
tests/comments.rs, or refactor tests to import a non-gated public entrypoint
instead of run::run_main; ensure you reference the run.rs module and the
run_main symbol when applying the chosen fix.

---

Outside diff comments:
In `@cpp-linter/src/cli/structs.rs`:
- Around line 246-289: The failing compile is caused by using Clap-only symbols
when the "bin" feature is disabled; wrap the Clap-dependent import and tests
with the feature gate: move or annotate use super::{Cli, LinesChangedOnly,
ThreadComments} so that Cli is only imported under #[cfg(feature = "bin")] and
add #[cfg(feature = "bin")] to the parse_positional,
display_lines_changed_only_enum, and display_thread_comments_enum tests (or
alternatively gate the entire test module with #[cfg(feature = "bin")]) so that
LinesChangedOnly::from_str and ThreadComments::from_str are only referenced when
the "bin" feature is enabled.

In `@cpp-linter/src/rest_api/mod.rs`:
- Around line 146-165: The make_format_comment and make_tidy_comment calls are
passing the swapped version fields; change the arguments so make_format_comment
receives clang_versions.format_version.as_ref().unwrap().to_string() and
make_tidy_comment receives
clang_versions.tidy_version.as_ref().unwrap().to_string(), keeping the other
parameters (files, &mut comment, *_checks_failed, &mut remaining_length)
unchanged so the correct tool versions are shown.

---

Minor comments:
In @.github/workflows/run-dev-tests.yml:
- Around line 107-117: The CI is running eval_version tests twice because the
"ci" profile's default-filter still includes eval_version; update the "ci"
profile's default-filter (the default-filter setting for the ci profile) to
exclude eval_version tests by replacing the inclusion of test(#*eval_version)
with an exclusion (- test(#*eval_version)), matching how the "all" profile
intentionally excludes them so warmup + ci do not duplicate eval_version runs.

In `@cpp-linter/src/logger.rs`:
- Line 2: The lib export for the logger module is unconditional while the module
file itself is gated with #![cfg(feature = "bin")]; update the module export so
it is also feature-gated by changing the `pub mod logger;` export to be
annotated with the same cfg (i.e., add `#[cfg(feature = "bin")]` before the `pub
mod logger;`) so the compiler only exports the logger when the `bin` feature is
enabled.

---

Nitpick comments:
In @.github/workflows/run-dev-tests.yml:
- Line 34: The matrix OS entries under the "os" key are inconsistently quoted;
update the matrix so both entries use the same quoting style (either both quoted
or both unquoted) for consistency and readability—locate the "os" matrix line in
the workflow (os: ["windows-latest", ubuntu-latest]) and make the two entries
uniform, e.g., change ubuntu-latest to "ubuntu-latest" to match
"windows-latest".

In `@clang-installer/Cargo.toml`:
- Around line 21-38: Add an inline comment in Cargo.toml next to the blake2 =
"0.10.6" and sha2 = "0.10.9" dependency entries explaining that these crypto
crates are intentionally pinned locally (not in workspace.dependencies) for
cryptographic stability/security and to avoid accidental upgrades that could
change hashing outputs or break binary compatibility; reference the blake2 and
sha2 lines so maintainers understand the deliberate pinning and know not to move
them to workspace.dependencies without review.

In `@clang-installer/README.md`:
- Around line 5-7: Replace the awkward sentence "This is a utility for
cpp-linter CLI, thus its API is considered internal to cpp-linter crate." with a
tighter phrasing such as: "This utility is used by the cpp-linter CLI, and its
API is internal to the `cpp-linter` crate." Update the README.md sentence to
match that suggested wording for clarity and grammar.

In `@clang-installer/src/downloader/caching.rs`:
- Around line 29-43: StaticDistDownloader currently bypasses the shared cache
validation logic by directly calling .exists() on the cache file; update
StaticDistDownloader (the code in StaticDistDownloader::... where it checks
cache_file.exists()) to call the shared helper is_cache_valid(cache_file,
max_age) instead so it honors the same max_age/eviction semantics as
is_cache_valid; ensure you pass the same max_age used elsewhere and remove the
direct .exists() check so future cache policy changes apply consistently.

In `@clang-installer/src/main.rs`:
- Around line 120-122: Add a clap value parser for the `directory` argument that
accepts non-existent paths and existing directories but rejects existing files;
implement a function like `validate_install_dir` (signature returning
Result<PathBuf, String> or implementing TypedValueParser) which converts the
input to a PathBuf, returns Ok for paths that don't exist or that are
directories, and returns Err with a clear message when the path exists and is a
file, then attach it to the `pub directory: Option<PathBuf>` arg via the
`#[arg(value_parser = validate_install_dir)]` attribute so validation happens in
the CLI layer instead of later in downloader/tool lookup code.

In `@clang-installer/src/tool.rs`:
- Around line 185-191: The current code uses fs::read_link(symlink_path) and
logs the link but doesn't verify the link's target exists or is accessible;
update the logic around symlink_path and the symlink variable to resolve the
final target (e.g., join symlink_path.parent() with symlink) and then call
fs::metadata or Path::exists on that resolved target to ensure the file exists
(and optionally is executable) before proceeding; keep the existing log but add
an error path that handles broken symlinks when fs::metadata(target) fails.

In `@clang-installer/tests/pypi.rs`:
- Around line 45-48: The test currently hides assertions inside setup(), so
update the code so setup() only performs initialization and move the assertions
into the test function download_clang_format; locate the async fn setup() and
remove any assert!/assert_eq!/expect calls from it, then call setup().await
inside download_clang_format and add the relevant assertions there (or split
setup into setup() and verify_* helpers if preferred) so the test function
explicitly contains the verification logic.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b2faad7e-a64b-42f1-97e3-435fd6ff482f

📥 Commits

Reviewing files that changed from the base of the PR and between b3a999d and 424a837.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (46)
  • .config/nextest.toml
  • .github/workflows/benchmark.yml
  • .github/workflows/binary-builds.yml
  • .github/workflows/run-dev-tests.yml
  • .pre-commit-config.yaml
  • Cargo.toml
  • bindings/node/Cargo.toml
  • bindings/node/src/lib.rs
  • bindings/python/Cargo.toml
  • clang-installer/Cargo.toml
  • clang-installer/README.md
  • clang-installer/src/downloader/caching.rs
  • clang-installer/src/downloader/hashing.rs
  • clang-installer/src/downloader/mod.rs
  • clang-installer/src/downloader/native_packages/mod.rs
  • clang-installer/src/downloader/native_packages/unix.rs
  • clang-installer/src/downloader/native_packages/windows.rs
  • clang-installer/src/downloader/pypi.rs
  • clang-installer/src/downloader/static_dist.rs
  • clang-installer/src/lib.rs
  • clang-installer/src/main.rs
  • clang-installer/src/progress_bar.rs
  • clang-installer/src/tool.rs
  • clang-installer/src/utils.rs
  • clang-installer/src/version.rs
  • clang-installer/tests/common.rs
  • clang-installer/tests/pypi.rs
  • clang-installer/tests/pypi_clang-format.json
  • clang-installer/tests/static_dist.rs
  • cpp-linter/Cargo.toml
  • cpp-linter/README.md
  • cpp-linter/src/clang_tools/clang_tidy.rs
  • cpp-linter/src/clang_tools/mod.rs
  • cpp-linter/src/cli/mod.rs
  • cpp-linter/src/cli/structs.rs
  • cpp-linter/src/common_fs/file_filter.rs
  • cpp-linter/src/common_fs/mod.rs
  • cpp-linter/src/logger.rs
  • cpp-linter/src/rest_api/github/mod.rs
  • cpp-linter/src/rest_api/mod.rs
  • cpp-linter/src/run.rs
  • cpp-linter/tests/comments.rs
  • cpp-linter/tests/demo/.clang-tidy
  • cspell.config.yml
  • docs/Cargo.toml
  • nurfile
💤 Files with no reviewable changes (2)
  • cpp-linter/tests/demo/.clang-tidy
  • cpp-linter/README.md

@2bndy5
Copy link
Copy Markdown
Collaborator Author

2bndy5 commented Mar 30, 2026

Eeesh! Coderabbit came pretty close to wiping the floor with my laziness/negligence. I'll come back to this soon/tomorrow. I need to nurture my broken ego first. 🤣


New additional CLI

cargo run --bin clang-tools --features bin -- --help

Usage: clang-tools.exe [OPTIONS]

Options:
  -v, --version [<VERSION>]
          The desired version of clang to install

          [default: ""]

  -t, --tool <TOOL>
          The clang tool to install

          [default: "clang-format clang-tidy"]
          [possible values: clang-tidy, clang-format]

  -d, --directory <DIRECTORY>
          The directory where the clang tools should be installed

  -f, --force
          Force overwriting symlink to the installed binary.

          This will only overwrite an existing symlink.

  -h, --help
          Print help (see a summary with '-h')

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

♻️ Duplicate comments (1)
bindings/node/index.d.ts (1)

3-3: ⚠️ Potential issue | 🟡 Minor

Please switch the Node binding test to a no-throw assertion.

Line 3 makes the public contract void, but bindings/node/__test__/index.spec.mjs still asserts await main(...) === 0. That expectation is now stale.

Based on learnings: In bindings/node/src/lib.rs, the #[napi] pub fn main was intentionally changed from pub async fn to pub fn (synchronous) because a newer type used in the call chain is not Send-compatible, requiring a block_on approach with an explicit Tokio runtime. The TypeScript binding should reflect a synchronous (non-Promise) return type.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@bindings/node/index.d.ts` at line 3, The test expects a numeric return but
the TypeScript binding now declares main(args: string[]) => void; update the
test in bindings/node/__test__/index.spec.mjs to use a no-throw assertion
instead of asserting await main(...) === 0 — call main([...]) synchronously (or
without awaiting) and assert it does not throw (e.g., wrap in try/catch or use
your test framework’s doesNotThrow/doesNotReject equivalent), keeping the
declaration in bindings/node/index.d.ts as export declare function main(args:
Array<string>): void.
🧹 Nitpick comments (2)
clang-installer/src/progress_bar.rs (1)

68-72: Add drop-time cleanup so early-return paths don’t leave the progress line unterminated.

finish() at Line 69 is manual. Callers that return early with ? can skip it, leaving a partial progress line. A best-effort Drop cleanup avoids noisy logs on error paths.

Proposed refactor
 pub struct ProgressBar {
@@
     prompt: String,
+    finished: bool,
 }
@@
         Self {
@@
             prompt: prompt.to_string(),
+            finished: false,
         }
     }
@@
     pub fn finish(&mut self) -> Result<()> {
         writeln!(&mut self.stdout_handle)?; // Move to the next line after finishing
-        self.stdout_handle.flush()
+        self.finished = true;
+        self.stdout_handle.flush()
     }
 }
+
+impl Drop for ProgressBar {
+    fn drop(&mut self) {
+        if !self.finished {
+            let _ = writeln!(&mut self.stdout_handle);
+            let _ = self.stdout_handle.flush();
+        }
+    }
+}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@clang-installer/src/progress_bar.rs` around lines 68 - 72, The progress line
can remain unterminated if callers return early and never call
ProgressBar::finish, so add a best-effort Drop implementation for the
ProgressBar type that ensures the line is terminated and flushed: add a
finished: bool field to the ProgressBar struct, set finished = true inside
finish(&mut self) after the writeln!/flush succeed, and implement Drop for
ProgressBar to check finished and if false perform a safe writeln! to
stdout_handle and a flush, swallowing/ignoring any errors (don't panic) to avoid
noisy logs on error paths; reference ProgressBar, finish, stdout_handle, and the
new finished flag when making the changes.
clang-installer/src/downloader/native_packages/windows.rs (1)

167-168: Redundant cmd variable shadowing.

Line 149 creates let mut cmd = Command::new(self.as_str()), but line 168 shadows it with let mut cmd = Command::new("winget"). The first cmd is unused in the Winget branch. While not a bug, it's cleaner to create cmd inside each match arm.

Optional cleanup
     async fn install_package(
         &self,
         package_name: &str,
         version: Option<&Version>,
     ) -> Result<(), PackageManagerError> {
-        let mut cmd = Command::new(self.as_str());
         match self {
             Self::Chocolatey => {
+                let mut cmd = Command::new(self.as_str());
                 cmd.arg("install").arg(package_name).arg("-y");
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@clang-installer/src/downloader/native_packages/windows.rs` around lines 167 -
168, The code currently creates a top-level let mut cmd =
Command::new(self.as_str()) and then shadows it with let mut cmd =
Command::new("winget") inside the Self::Winget match arm; remove the redundant
top-level creation and instead instantiate Command::new(...) inside each match
arm (e.g., use Command::new(self.as_str()) in the arms that need it and
Command::new("winget") inside the Self::Winget arm) so the variable cmd is only
declared once per arm and there is no shadowing of cmd or unused bindings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@clang-installer/src/downloader/native_packages/windows.rs`:
- Around line 107-124: The winget parsing loop incorrectly skips an extra token
before reading the version: in the for loop where lines are split into l
(split_ascii_whitespace().skip(1)) and you check package_name, remove the extra
l.next() that skips the version; instead after confirming name == package_name
and consuming the id via l.next() for the id check, directly call l.next() once
to get ver_str and parse it with Version::parse (used with
ver_cmp.matches(&ver)); update the logic around ver_cmp.clone() / Version::parse
usage so the version token is read immediately after the id-consumption and
treat missing/invalid version as not installed as before.

In `@clang-installer/src/progress_bar.rs`:
- Around line 63-64: The inc method and the step-calculation code must be
hardened: in ProgressBar::inc use checked_add or saturating_add to update
self.current and then clamp self.current to at most self.total to avoid overflow
and runaway values; when computing ratios (current/total) treat total == 0
specially (e.g., treat total as 1 or mark complete) and compute a bounded ratio
in [0.0,1.0]; compute new_steps = (ratio * max_steps) but then cap new_steps to
a reasonable range (e.g., 0..=max_steps) before entering the render loops so the
loops at the locations that use new_steps (the bodies referenced around the
render loops at Line 91 and Line 106) cannot iterate extremely many times. Apply
these same clamping/guard patterns to all places that use total, current, ratio,
or new_steps so the progress bar cannot drive pathological render loops.

In `@clang-installer/src/version.rs`:
- Around line 117-135: The cache lookup builds cached_bin using
get_min_ver(version_req) which yields a full semver (e.g., "12.0.0") but
PyPI-stored binaries use a major-only name (see pypi.rs around the PyPI download
naming), so the code in the StaticDistDownloader::get_cache_dir() -> join("bin")
path misses PyPI entries; update the lookup in the function that builds
cached_bin to also try the major-only variant (e.g., format
"{tool}-{min_ver.major}{bin_ext}") or normalize both producers to the same
filename scheme, attempt both candidate paths, and when a candidate exists call
tool.capture_version(&candidate) and validate with version_req.matches(&version)
before returning the ClangVersion with the matched path.

---

Duplicate comments:
In `@bindings/node/index.d.ts`:
- Line 3: The test expects a numeric return but the TypeScript binding now
declares main(args: string[]) => void; update the test in
bindings/node/__test__/index.spec.mjs to use a no-throw assertion instead of
asserting await main(...) === 0 — call main([...]) synchronously (or without
awaiting) and assert it does not throw (e.g., wrap in try/catch or use your test
framework’s doesNotThrow/doesNotReject equivalent), keeping the declaration in
bindings/node/index.d.ts as export declare function main(args: Array<string>):
void.

---

Nitpick comments:
In `@clang-installer/src/downloader/native_packages/windows.rs`:
- Around line 167-168: The code currently creates a top-level let mut cmd =
Command::new(self.as_str()) and then shadows it with let mut cmd =
Command::new("winget") inside the Self::Winget match arm; remove the redundant
top-level creation and instead instantiate Command::new(...) inside each match
arm (e.g., use Command::new(self.as_str()) in the arms that need it and
Command::new("winget") inside the Self::Winget arm) so the variable cmd is only
declared once per arm and there is no shadowing of cmd or unused bindings.

In `@clang-installer/src/progress_bar.rs`:
- Around line 68-72: The progress line can remain unterminated if callers return
early and never call ProgressBar::finish, so add a best-effort Drop
implementation for the ProgressBar type that ensures the line is terminated and
flushed: add a finished: bool field to the ProgressBar struct, set finished =
true inside finish(&mut self) after the writeln!/flush succeed, and implement
Drop for ProgressBar to check finished and if false perform a safe writeln! to
stdout_handle and a flush, swallowing/ignoring any errors (don't panic) to avoid
noisy logs on error paths; reference ProgressBar, finish, stdout_handle, and the
new finished flag when making the changes.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 63d61d73-4af8-4506-8131-b0b41e23e157

📥 Commits

Reviewing files that changed from the base of the PR and between 424a837 and 65e08a7.

📒 Files selected for processing (12)
  • bindings/node/index.d.ts
  • bindings/node/src/lib.rs
  • clang-installer/Cargo.toml
  • clang-installer/src/downloader/hashing.rs
  • clang-installer/src/downloader/mod.rs
  • clang-installer/src/downloader/native_packages/mod.rs
  • clang-installer/src/downloader/native_packages/unix.rs
  • clang-installer/src/downloader/native_packages/windows.rs
  • clang-installer/src/downloader/pypi.rs
  • clang-installer/src/downloader/static_dist.rs
  • clang-installer/src/progress_bar.rs
  • clang-installer/src/version.rs
✅ Files skipped from review due to trivial changes (1)
  • clang-installer/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (2)
  • clang-installer/src/downloader/mod.rs
  • clang-installer/src/downloader/native_packages/mod.rs

@2bndy5 2bndy5 marked this pull request as ready for review April 14, 2026 23:54
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (5)
clang-installer/src/version.rs (1)

338-340: Commented-out assertion in eval_version test.

Line 339 has a commented assertion:

// assert!(version_req.matches(&clang_path.version));

If this check is expected to pass but currently doesn't, it may indicate a bug in the version resolution logic. If it's intentionally disabled, consider adding a comment explaining why, or remove the dead code.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@clang-installer/src/version.rs` around lines 338 - 340, The commented-out
assertion in the eval_version test (the line with //
assert!(version_req.matches(&clang_path.version));) should be either reinstated
and fixed or removed with an explanatory comment: locate the eval_version test
and the variables version_req and clang_path, run the test to see if
version_req.matches(&clang_path.version) actually fails; if it fails, correct
the version resolution logic (where eval_version constructs
clang_path/version_req) so the match holds and re-enable the assertion,
otherwise remove the dead assert and add a short comment explaining why the
check is intentionally disabled.
clang-installer/src/downloader/mod.rs (1)

83-96: chmod_file opens file for write unnecessarily.

The function opens the file with write access just to read metadata and set permissions. This can be simplified using fs::set_permissions directly:

Simplified implementation
 #[cfg(unix)]
 fn chmod_file(path: &Path, mode: Option<u32>) -> std::io::Result<()> {
-    // Make the extracted binary executable on Unix-like systems.
     use std::os::unix::fs::PermissionsExt;
-    let out = fs::OpenOptions::new().write(true).open(path)?;
-    let mut perms = out.metadata()?.permissions();
+    let mut perms = fs::metadata(path)?.permissions();
     match mode {
         Some(mode) => {
             let prev = perms.mode();
             perms.set_mode(prev | mode);
         }
         None => perms.set_mode(0o755),
     }
-    out.set_permissions(perms)
+    fs::set_permissions(path, perms)
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@clang-installer/src/downloader/mod.rs` around lines 83 - 96, The function
chmod_file currently opens the file with
fs::OpenOptions::new().write(true).open(path) just to read metadata and change
permissions; remove that unnecessary open and instead use path.metadata() (or
fs::metadata(path)) to read current permissions and fs::set_permissions(path,
perms) to write them. Keep the same logic using
std::os::unix::fs::PermissionsExt to compute new mode (use perms.mode(),
set_mode(prev | mode) or default 0o755) but replace references to out.metadata()
and out.set_permissions(...) with metadata() and fs::set_permissions(path,
perms) so the file is not opened for writing.
clang-installer/src/downloader/native_packages/windows.rs (2)

42-47: is_installed() succeeds even when the package manager exits with error.

Command::output().is_ok() returns true whenever the process spawns successfully, regardless of its exit code. A broken or partially installed package manager that exits non-zero will still be considered "installed".

Consider checking status.success():

Suggested fix
     fn is_installed(&self) -> bool {
         Command::new(self.as_str())
             .arg("--version")
             .output()
-            .is_ok()
+            .is_ok_and(|out| out.status.success())
     }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@clang-installer/src/downloader/native_packages/windows.rs` around lines 42 -
47, The is_installed() method currently treats any spawned process as success
because it uses Command::output().is_ok(); change it to capture the process
result (e.g., let out = Command::new(self.as_str()).arg("--version").output())
and return whether the process exited successfully by checking
out.status.success() (or use Command::status() and map(|s| s.success())). Update
the is_installed() function to return that boolean instead of .is_ok().

165-173: Redundant cmd variable shadow in Winget branch.

Line 147 creates cmd from self.as_str(), but the Winget branch at line 166 immediately shadows it with a new Command::new("winget"). The original cmd is never used in this branch.

Minor cleanup
             Self::Winget => {
-                let mut cmd = Command::new("winget");
-                cmd.arg("install").arg("--id").arg(package_name);
+                cmd.arg("install").arg("--id").arg(package_name);
                 if let Some(version) = version {
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@clang-installer/src/downloader/native_packages/windows.rs` around lines 165 -
173, The Winget branch shadows the earlier-created `cmd` (created from
`self.as_str()`) by doing `let mut cmd = Command::new("winget")`, leaving the
original `cmd` unused; fix by removing the shadowing and reusing the existing
`cmd` (or alternatively move the `Command::new(...)` creation into each branch
and delete the earlier `cmd` creation) so that the `Self::Winget` arm uses the
same `cmd` variable rather than creating a new one; update the `Self::Winget`
branch that currently references `Command::new("winget")` to use the existing
`cmd` variable and ensure `cmd.arg(...)` calls remain correct.
clang-installer/src/downloader/pypi.rs (1)

470-475: Size-only check for cached binary could miss content corruption.

The extraction skips re-extracting if meta.len() == file.size(), but this doesn't detect corruption where the file size remains the same. Since the wheel is verified by digest before extraction, this is low-risk, but a hash check would be more robust.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@clang-installer/src/downloader/pypi.rs` around lines 470 - 475, The
cached-binary validation currently only compares fs::metadata(...).len() against
file.size() (extracted_bin, meta.len(), file.size()), which can miss same-size
corruption; update the check to compute and verify a content digest of
extracted_bin (using the same hash routine used earlier for the wheel) and
compare it to the expected digest before skipping extraction, falling back to
re-extract if the hash is missing or mismatches; if computing the digest is
expensive, at minimum keep the size check but additionally compute a SHA256 (or
existing digest function) of extracted_bin and compare to the wheel's verified
digest to ensure integrity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@clang-installer/src/progress_bar.rs`:
- Around line 66-68: The inc method in ProgressBar incorrectly doubles current
by doing self.current += self.current.saturating_add(delta);; change it to add
only delta using saturating_add (e.g., assign self.current =
self.current.saturating_add(delta) or use self.current =
self.current.saturating_add(delta);) so ProgressBar::inc updates linearly, then
call self.render() as before; update the implementation inside the inc(&mut
self, delta: u64) -> Result<()> function.
- Around line 117-142: The else branch currently treats both "advance_bar is
None" (unknown total) and "advance_bar is Some(new_steps) but new_steps <=
self.steps" the same, causing unwanted increments when total is known but
progress didn't advance; update the logic around advance_bar/self.total in the
progress update function so that: if advance_bar is None (unknown total) then
increment self.steps and render the bar as before, but if advance_bar is
Some(new_steps) and new_steps <= self.steps do nothing (no increment, no
write/flush); only render/increment for the known-total case when new_steps >
self.steps. Adjust control flow around the symbols advance_bar, self.total,
self.steps, is_interactive, stdout_handle, LOG_INDENT, BAR_CHAR and prompt to
reflect this separation.

---

Nitpick comments:
In `@clang-installer/src/downloader/mod.rs`:
- Around line 83-96: The function chmod_file currently opens the file with
fs::OpenOptions::new().write(true).open(path) just to read metadata and change
permissions; remove that unnecessary open and instead use path.metadata() (or
fs::metadata(path)) to read current permissions and fs::set_permissions(path,
perms) to write them. Keep the same logic using
std::os::unix::fs::PermissionsExt to compute new mode (use perms.mode(),
set_mode(prev | mode) or default 0o755) but replace references to out.metadata()
and out.set_permissions(...) with metadata() and fs::set_permissions(path,
perms) so the file is not opened for writing.

In `@clang-installer/src/downloader/native_packages/windows.rs`:
- Around line 42-47: The is_installed() method currently treats any spawned
process as success because it uses Command::output().is_ok(); change it to
capture the process result (e.g., let out =
Command::new(self.as_str()).arg("--version").output()) and return whether the
process exited successfully by checking out.status.success() (or use
Command::status() and map(|s| s.success())). Update the is_installed() function
to return that boolean instead of .is_ok().
- Around line 165-173: The Winget branch shadows the earlier-created `cmd`
(created from `self.as_str()`) by doing `let mut cmd = Command::new("winget")`,
leaving the original `cmd` unused; fix by removing the shadowing and reusing the
existing `cmd` (or alternatively move the `Command::new(...)` creation into each
branch and delete the earlier `cmd` creation) so that the `Self::Winget` arm
uses the same `cmd` variable rather than creating a new one; update the
`Self::Winget` branch that currently references `Command::new("winget")` to use
the existing `cmd` variable and ensure `cmd.arg(...)` calls remain correct.

In `@clang-installer/src/downloader/pypi.rs`:
- Around line 470-475: The cached-binary validation currently only compares
fs::metadata(...).len() against file.size() (extracted_bin, meta.len(),
file.size()), which can miss same-size corruption; update the check to compute
and verify a content digest of extracted_bin (using the same hash routine used
earlier for the wheel) and compare it to the expected digest before skipping
extraction, falling back to re-extract if the hash is missing or mismatches; if
computing the digest is expensive, at minimum keep the size check but
additionally compute a SHA256 (or existing digest function) of extracted_bin and
compare to the wheel's verified digest to ensure integrity.

In `@clang-installer/src/version.rs`:
- Around line 338-340: The commented-out assertion in the eval_version test (the
line with // assert!(version_req.matches(&clang_path.version));) should be
either reinstated and fixed or removed with an explanatory comment: locate the
eval_version test and the variables version_req and clang_path, run the test to
see if version_req.matches(&clang_path.version) actually fails; if it fails,
correct the version resolution logic (where eval_version constructs
clang_path/version_req) so the match holds and re-enable the assertion,
otherwise remove the dead assert and add a short comment explaining why the
check is intentionally disabled.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: a0ee8e6a-a223-4f0f-b8a6-827f5901f23f

📥 Commits

Reviewing files that changed from the base of the PR and between 65e08a7 and 0a3f1f3.

📒 Files selected for processing (7)
  • .github/workflows/run-dev-tests.yml
  • clang-installer/src/downloader/mod.rs
  • clang-installer/src/downloader/native_packages/windows.rs
  • clang-installer/src/downloader/pypi.rs
  • clang-installer/src/downloader/static_dist.rs
  • clang-installer/src/progress_bar.rs
  • clang-installer/src/version.rs
🚧 Files skipped from review as they are similar to previous changes (2)
  • .github/workflows/run-dev-tests.yml
  • clang-installer/src/downloader/static_dist.rs

@2bndy5 2bndy5 force-pushed the clang-tools-downloader branch from 0a3f1f3 to 2ad97e4 Compare April 15, 2026 00:30
@2bndy5 2bndy5 force-pushed the clang-tools-downloader branch from 2ad97e4 to 068f1ff Compare April 15, 2026 00:37
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

♻️ Duplicate comments (3)
clang-installer/src/downloader/pypi.rs (1)

440-449: ⚠️ Potential issue | 🟠 Major

Major-version output names can reuse the wrong patch build.

bin_name is keyed only by ver.major, and extract_bin() skips rewriting whenever an existing file has the same size. If two patch releases in the same major ship same-sized binaries, a later request can return the older executable. Key the extracted binary by the full version, or make the reuse check compare stronger identity than file length.

Suggested fix
         let bin_name = format!(
-            "{clang_tool}-{}{}",
-            ver.major,
+            "{clang_tool}-{ver}{}",
             if cfg!(windows) { ".exe" } else { "" }
         );

Also applies to: 469-474

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@clang-installer/src/downloader/pypi.rs` around lines 440 - 449, The code
currently constructs bin_name using only ver.major which can reuse a same-sized
binary from a different patch; update the naming or identity check so extracted
binaries are unique per full version: change the bin_name format in the block
that builds bin_name (referencing ver.major, bin_name, extracted_bin,
cached_wheel and the call Self::extract_bin) to include the full version (e.g.,
ver.to_string() or ver.major,ver.minor,ver.patch) instead of just ver.major, or
alternately modify extract_bin to validate reuse by a stronger identity (e.g.,
compare a checksum/sha256 of cached_wheel vs existing file) before skipping
extraction so different patch builds with identical sizes won’t be incorrectly
reused.
clang-installer/src/downloader/mod.rs (1)

61-73: ⚠️ Potential issue | 🟠 Major

Create the temp file in the cache directory before persist().

NamedTempFile::new() uses the process temp dir, and persist() cannot move that file across filesystems. If CPP_LINTER_CACHE points at another mount, the download can succeed and still fail at the final persist. Use NamedTempFile::new_in(cache_parent) (or tempfile::Builder::tempfile_in) so the rename stays on the same filesystem. (docs.rs)

Suggested fix
-    let mut tmp_file = tempfile::NamedTempFile::new()?;
+    let mut tmp_file = if let Some(cache_parent) = cache_path.parent() {
+        tempfile::NamedTempFile::new_in(cache_parent)?
+    } else {
+        tempfile::NamedTempFile::new()?
+    };
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@clang-installer/src/downloader/mod.rs` around lines 61 - 73, The temp file is
created with NamedTempFile::new() in another filesystem so
tmp_file.persist(cache_path) can fail; instead create the temp file inside the
cache directory before writing so the final rename stays on the same filesystem.
Replace NamedTempFile::new() with creating the temp file in the cache parent
directory (e.g., NamedTempFile::new_in(cache_parent) or
tempfile::Builder::tempfile_in) using the parent of cache_path, then continue
writing, flushing, setting modified time, and calling
tmp_file.persist(cache_path) as before; ensure you handle the case where
cache_path.parent() is None or missing by creating the parent directory first.
clang-installer/src/progress_bar.rs (1)

79-90: ⚠️ Potential issue | 🟡 Minor

render() still no-ops at 0% for known totals.

When current == 0, new_steps is also 0, so the new_steps > self.steps guard skips the initial draw entirely. That makes the documented “render 0% state” path misleading, and the eager progress_bar.render() call in clang-installer/src/downloader/mod.rs won’t show anything until the first visible step.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@clang-installer/src/progress_bar.rs` around lines 79 - 90, The initial 0%
state is skipped because when current == 0 you get new_steps == 0 and the
existing guard only draws on new_steps > self.steps; update the guard in
ProgressBar::render to also allow the initial draw when steps == 0 for known
totals. Concretely, change the if-let condition that matches advance_bar to
check (new_steps > self.steps) || (self.steps == 0 && self.current == 0) so
render() will draw the 0% bar for the first call (refer to method render, fields
current, steps, total, and constant MAX_BAR_WIDTH).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@clang-installer/src/downloader/pypi.rs`:
- Around line 427-449: The cache-population code around checking is_cache_valid,
downloading to cached_wheel, and extracting to extracted_bin has a race between
processes; wrap the entire "check-then-act" block (the path that performs
download(&Url::parse(&info.url)?, &cached_wheel, 60).await? and
Self::extract_bin(...)) in a per-artifact file lock so only one process
populates a given cached_wheel/extracted_bin at a time: create a lock file
(e.g., cached_wheel.with_extension("lock") or similar) and acquire an advisory
exclusive lock before re-checking is_cache_valid, then perform download and
digest.verify and extract_bin, then release the lock; also apply the same lock
pattern to the other cache-population site referenced in the comment (the block
around lines 469-484) to prevent concurrent downloads/extractions.
- Around line 123-133: is_compatible_with_system currently returns true based
only on target_env and ignores the version stored in LinuxLibC, so wheels with
higher libc requirements can be accepted; update the
LinuxLibC::is_compatible_with_system method to (1) extract the required
major/minor version from the enum variants (LinuxLibC::Glibc and
LinuxLibC::Musl), (2) detect the runtime libc version at startup (e.g., call the
platform-appropriate runtime detection for glibc and musl or fall back to
parsing ldd/ldd --version output) and (3) compare runtime_major:runtime_minor >=
required_major:required_minor and return false if the runtime is older or
detection fails; ensure the code paths in is_compatible_with_system handle both
Glibc and Musl variants and return false for unknown/unsupported cases.

In `@clang-installer/src/progress_bar.rs`:
- Around line 40-45: The doctest passes an Option<u64> to ProgressBar::new but
the signature expects Option<NonZero<u64>>; update the example to construct a
NonZero<u64> and pass it as Some(NonZero::new(...).unwrap()). Specifically
modify the example around ProgressBar::new to import or reference
std::num::NonZero and call ProgressBar::new(Some(NonZero::new(100).unwrap()),
"Downloading") so the doctest compiles with the correct type for
ProgressBar::new.

---

Duplicate comments:
In `@clang-installer/src/downloader/mod.rs`:
- Around line 61-73: The temp file is created with NamedTempFile::new() in
another filesystem so tmp_file.persist(cache_path) can fail; instead create the
temp file inside the cache directory before writing so the final rename stays on
the same filesystem. Replace NamedTempFile::new() with creating the temp file in
the cache parent directory (e.g., NamedTempFile::new_in(cache_parent) or
tempfile::Builder::tempfile_in) using the parent of cache_path, then continue
writing, flushing, setting modified time, and calling
tmp_file.persist(cache_path) as before; ensure you handle the case where
cache_path.parent() is None or missing by creating the parent directory first.

In `@clang-installer/src/downloader/pypi.rs`:
- Around line 440-449: The code currently constructs bin_name using only
ver.major which can reuse a same-sized binary from a different patch; update the
naming or identity check so extracted binaries are unique per full version:
change the bin_name format in the block that builds bin_name (referencing
ver.major, bin_name, extracted_bin, cached_wheel and the call Self::extract_bin)
to include the full version (e.g., ver.to_string() or
ver.major,ver.minor,ver.patch) instead of just ver.major, or alternately modify
extract_bin to validate reuse by a stronger identity (e.g., compare a
checksum/sha256 of cached_wheel vs existing file) before skipping extraction so
different patch builds with identical sizes won’t be incorrectly reused.

In `@clang-installer/src/progress_bar.rs`:
- Around line 79-90: The initial 0% state is skipped because when current == 0
you get new_steps == 0 and the existing guard only draws on new_steps >
self.steps; update the guard in ProgressBar::render to also allow the initial
draw when steps == 0 for known totals. Concretely, change the if-let condition
that matches advance_bar to check (new_steps > self.steps) || (self.steps == 0
&& self.current == 0) so render() will draw the 0% bar for the first call (refer
to method render, fields current, steps, total, and constant MAX_BAR_WIDTH).
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 06ed31f9-fc8e-45c5-8518-b6eeae48862a

📥 Commits

Reviewing files that changed from the base of the PR and between 0a3f1f3 and 03225d5.

📒 Files selected for processing (4)
  • benchmark/benchmark.nu
  • clang-installer/src/downloader/mod.rs
  • clang-installer/src/downloader/pypi.rs
  • clang-installer/src/progress_bar.rs

Comment on lines +123 to +133
impl LinuxLibC {
/// Checks if the [LinuxLibC] is compatible with the current system.
pub fn is_compatible_with_system(&self) -> bool {
match self {
#[cfg(target_env = "musl")]
LinuxLibC::Musl { .. } => true,
#[cfg(not(target_env = "musl"))]
LinuxLibC::Glibc { .. } => true,
_ => false,
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

find . -type f -name "pypi.rs" | head -20

Repository: cpp-linter/cpp-linter-rs

Length of output: 140


🏁 Script executed:

wc -l ./clang-installer/src/downloader/pypi.rs

Repository: cpp-linter/cpp-linter-rs

Length of output: 112


🏁 Script executed:

sed -n '1,50p' ./clang-installer/src/downloader/pypi.rs

Repository: cpp-linter/cpp-linter-rs

Length of output: 1679


🏁 Script executed:

sed -n '60,160p' ./clang-installer/src/downloader/pypi.rs

Repository: cpp-linter/cpp-linter-rs

Length of output: 3212


🏁 Script executed:

sed -n '140,160p' ./clang-installer/src/downloader/pypi.rs

Repository: cpp-linter/cpp-linter-rs

Length of output: 752


🏁 Script executed:

sed -n '146,200p' ./clang-installer/src/downloader/pypi.rs

Repository: cpp-linter/cpp-linter-rs

Length of output: 2004


🏁 Script executed:

sed -n '200,280p' ./clang-installer/src/downloader/pypi.rs

Repository: cpp-linter/cpp-linter-rs

Length of output: 2985


🏁 Script executed:

sed -n '280,350p' ./clang-installer/src/downloader/pypi.rs

Repository: cpp-linter/cpp-linter-rs

Length of output: 2504


🏁 Script executed:

sed -n '350,400p' ./clang-installer/src/downloader/pypi.rs

Repository: cpp-linter/cpp-linter-rs

Length of output: 2272


🏁 Script executed:

rg "is_compatible_with_system|version.*libc|libc.*version" ./clang-installer/src/downloader/ -A 3 -B 1

Repository: cpp-linter/cpp-linter-rs

Length of output: 3609


🏁 Script executed:

rg "glibc|musl" ./clang-installer/src/downloader/ -i | grep -i "version\|detect\|compat"

Repository: cpp-linter/cpp-linter-rs

Length of output: 437


🏁 Script executed:

sed -n '110,135p' ./clang-installer/src/downloader/pypi.rs

Repository: cpp-linter/cpp-linter-rs

Length of output: 761


Detect and enforce minimum glibc/musl version compatibility.

The LinuxLibC enum stores the version requirement from wheel tags (manylinux_x_y_*, musllinux_x_y_*), but is_compatible_with_system() ignores it. Wheels with higher version requirements will download and extract successfully but fail at runtime on older systems when calling unavailable libc functions. Either detect the runtime libc version and compare against the wheel's requirement, or restrict to explicitly supported minimum versions.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@clang-installer/src/downloader/pypi.rs` around lines 123 - 133,
is_compatible_with_system currently returns true based only on target_env and
ignores the version stored in LinuxLibC, so wheels with higher libc requirements
can be accepted; update the LinuxLibC::is_compatible_with_system method to (1)
extract the required major/minor version from the enum variants
(LinuxLibC::Glibc and LinuxLibC::Musl), (2) detect the runtime libc version at
startup (e.g., call the platform-appropriate runtime detection for glibc and
musl or fall back to parsing ldd/ldd --version output) and (3) compare
runtime_major:runtime_minor >= required_major:required_minor and return false if
the runtime is older or detection fails; ensure the code paths in
is_compatible_with_system handle both Glibc and Musl variants and return false
for unknown/unsupported cases.

Comment on lines +427 to +449
if Self::is_cache_valid(&cached_wheel, None) {
log::info!(
"Using cached wheel for {clang_tool} version {ver} from {}",
cached_wheel.to_string_lossy()
);
} else {
log::info!("Downloading {clang_tool} version {ver} from {}", info.url);
download(&Url::parse(&info.url)?, &cached_wheel, 60).await?;
}
if let Some(digest) = info.digests.first() {
log::info!("Verifying wheel file integrity with digest: {digest:?}");
digest.verify(&cached_wheel)?;
}
let bin_name = format!(
"{clang_tool}-{}{}",
ver.major,
if cfg!(windows) { ".exe" } else { "" }
);
let extracted_bin = match directory {
None => cached_dir.join(format!("bin/{bin_name}",)),
Some(dir) => dir.join(&bin_name),
};
Self::extract_bin(clang_tool, &cached_wheel, &extracted_bin)?;
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Apr 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Serialize cache population across processes.

This is still a check-then-act race on cached_wheel and extracted_bin: two processes can both miss the cache, then download/extract into the same paths concurrently. Since this repo’s tests run in separate processes under nextest, an in-memory mutex would not help here; this needs a per-artifact lock file around the whole cache-population section.
Based on learnings, this repo’s nextest setup runs each test in its own process.

Also applies to: 469-484

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@clang-installer/src/downloader/pypi.rs` around lines 427 - 449, The
cache-population code around checking is_cache_valid, downloading to
cached_wheel, and extracting to extracted_bin has a race between processes; wrap
the entire "check-then-act" block (the path that performs
download(&Url::parse(&info.url)?, &cached_wheel, 60).await? and
Self::extract_bin(...)) in a per-artifact file lock so only one process
populates a given cached_wheel/extracted_bin at a time: create a lock file
(e.g., cached_wheel.with_extension("lock") or similar) and acquire an advisory
exclusive lock before re-checking is_cache_valid, then perform download and
digest.verify and extract_bin, then release the lock; also apply the same lock
pattern to the other cache-population site referenced in the comment (the block
around lines 469-484) to prevent concurrent downloads/extractions.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This concern was addressed in the CI workflow. Before running the test suite, a new "warmup" profile is executed first. The "warmup" profile consists of 1 test: clang-installer::version::eval_version whihc populates the cache with the clang binaries of the version being tested.

For user experience, they can do something similar with the added clang-tools binary, which installs a specified version of the specified clang tools. They would just need to run clang-tools first before running cpp-linter in parallel on different projects.


I don't see a better way to protect against concurrent downloading/extracting for the same clang-tool version. I know Linux has a mechanism for such problems, but IDK about macOS or Windows.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

@2bndy5, the warmup-profile approach is a pragmatic and clean solution for the test suite — serializing cache population before the parallel test run effectively eliminates the race for CI purposes.

Regarding your question about cross-platform file locking: it is solvable in Rust without platform-specific code. Two well-established crates cover all three platforms (Linux, macOS, Windows):

  • fd-lock — advisory cross-platform lock on a file using a file descriptor to it. File locks are implemented with flock(2) on Unix and LockFile on Windows. It exposes an RwLock-style API for blocking write acquisition.
  • async-fd-lock — advisory cross-platform file locks using file descriptors, with async support by off-loading blocking operations to newly spawned blocking tasks. This would fit better since download_tool is already async.

The important caveat: advisory lock compliance is opt-in, and can freely be ignored by other parties — this means this crate should never be used for security purposes, but solely to coordinate file access. Since the only writer would be clang-installer itself, that's perfectly fine here.

If you ever want to harden the general user-facing case (e.g., two cpp-linter processes running in parallel on different projects), the pattern would be something like:

// acquire an exclusive write lock on a sentinel lock file before the check-then-act block
let lock_path = cached_wheel.with_extension("lock");
let lock_file = fs::OpenOptions::new().create(true).write(true).open(&lock_path)?;
let mut locked = async_fd_lock::RwLock::new(lock_file).write().await?;

// re-check cache validity inside the lock
if !Self::is_cache_valid(&cached_wheel, None) {
    download(&Url::parse(&info.url)?, &cached_wheel, 60).await?;
}
// digest verify + extract_bin still inside lock
drop(locked);

That said, the warmup-profile mitigation is sufficient for now, so this is just FYI if you decide to address the user-facing scenario in a future PR.

(◕ᴗ◕✿)


🌐 Web query:

Rust cross-platform advisory file locking crate fs2 fd-lock

💡 Result:

If you want cross-platform advisory (opt-in) whole-file locking in Rust, fs2 and fd-lock are both valid but have different tradeoffs:

  • fs2: adds a FileExt trait on std::fs::File with lock_shared, lock_exclusive, try_lock_*, and unlock. Locks are explicitly documented as advisory, with cross-platform caveats; implementation uses flock(2) on Unix and LockFile on Windows. fs2 is also quite old (latest 0.4.3 from 2018) and depends on libc/winapi. [1]

  • fd-lock: provides an advisory reader-writer lock wrapper (RwLock<T>) with RAII guards. API is closer to std::sync::RwLock: read(&self), try_read(&self), but write(&mut self) / try_write(&mut self) require &mut self (often a notable ergonomics difference). It uses rustix on Unix and windows-sys on Windows, and is actively versioned (e.g., 4.0.4 on docs.rs). [2][3]

Practical guidance:

  • Prefer fd-lock if you want a higher-level RW-lock + guard API and a more modern backend (rustix/windows-sys). [2][3]
  • Prefer fs2 if you specifically want the simple FileExt trait on std::fs::File and don’t mind the older crate/runtime deps. [1]
  • Also consider fs4 if you like fs2’s model but want a maintained fork aiming to support async and replace libc with rustix. [4]

Sources:
[1] fs2::FileExt docs (locking API, advisory notes, flock/LockFile) (docs.rs)
[2] fd-lock crate docs (advisory locks, overview) (docs.rs)
[3] fd-lock::RwLock docs (methods, write(&mut self)) (docs.rs)
[4] fs4-rs repo README (fork of fs2; async + rustix goal) (github.com)

Citations:


@2bndy5, the warmup-profile approach is a clean and pragmatic solution for the test-suite race — serializing cache population before the parallel run is exactly the right mitigation at the CI level.

On your question about cross-platform file locking: it is fully solvable in Rust on all three platforms. Two modern options:

  • fd-lock (v4.0.4, actively maintained) — provides an advisory reader-writer lock wrapper (RwLock<T>) with RAII guards; write(&mut self) / try_write(&mut self) require &mut self. It uses rustix on Unix and windows-sys on Windows.
  • fs4 — a maintained fork of fs2 aiming to support async and replace libc with rustix, which would fit nicely since download_tool is already async.

Note that fs2 (the original) is quite old — latest 0.4.3 from 2018 — and depends on libc/winapi, so it's better avoided for new code.

If you ever want to harden the general user-facing case (e.g., two cpp-linter processes populating the same cache concurrently), the fd-lock pattern would look roughly like:

// Create/open a sentinel lock file next to the cached artifact
let lock_path = cached_wheel.with_extension("lock");
let mut lock_file = fs::OpenOptions::new().create(true).write(true).open(&lock_path)?;
let _guard = fd_lock::RwLock::new(&mut lock_file).write()?;

// Re-check inside the lock (another process may have populated it while we waited)
if !Self::is_cache_valid(&cached_wheel, None) {
    download(&Url::parse(&info.url)?, &cached_wheel, 60).await?;
    digest.verify(&cached_wheel)?;
}
Self::extract_bin(clang_tool, &cached_wheel, &extracted_bin)?;
// _guard dropped here → lock released

That said, the warmup-profile mitigation is entirely sufficient for now. This is just FYI if you decide to harden the user-facing scenario in a future PR.

(◕ᴗ◕✿)


✏️ Learnings added
Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-rs PR: 279
File: clang-installer/src/downloader/pypi.rs:427-449
Timestamp: 2026-04-15T02:34:44.555Z
Learning: In `clang-installer/src/downloader/pypi.rs` and related cache-population code, the check-then-act race condition between parallel processes (nextest runs each test in its own process) is mitigated at the CI level via a `warmup` nextest profile (in `.config/nextest.toml`) that pre-populates the clang tool cache before the parallel test suite runs. The maintainer (2bndy5) intentionally chose this approach over in-code file locking for now. For cross-platform file locking in Rust, `fd-lock` (blocking) and `async-fd-lock` (async/tokio-compatible) are the recommended crates.

Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-rs PR: 279
File: clang-installer/src/downloader/pypi.rs:427-449
Timestamp: 2026-04-15T02:34:44.555Z
Learning: In `clang-installer/src/downloader/pypi.rs` and related cache-population code, the check-then-act race condition between parallel processes (nextest runs each test in its own process) is mitigated at the CI level via a `warmup` nextest profile (in `.config/nextest.toml`) that pre-populates the clang tool cache before the parallel test suite runs. The maintainer (2bndy5) intentionally chose this approach over in-code file locking. For cross-platform advisory file locking in Rust, `fd-lock` (v4.0.4, blocking RW-lock with `rustix`/`windows-sys` backend) and `fs4` (async-capable maintained fork of `fs2`) are the recommended crates.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


🧠 Learnings used
Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-rs PR: 279
File: clang-installer/src/version.rs:117-135
Timestamp: 2026-04-14T23:52:30.534Z
Learning: In `clang-installer/src/version.rs`, the cached binary lookup using `StaticDistDownloader::get_cache_dir()` is intentionally scoped only to StaticDistDownloader-cached binaries. It does NOT look for PyPI-downloaded binaries because: (1) the PyPI downloader runs first in the resolution chain before StaticDistDownloader, and (2) the SHA hash of a PyPI-extracted binary would not match what StaticDistDownloader expects. Mixing PyPI and StaticDistDownloader cached binaries is deliberately avoided.

Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-rs PR: 279
File: clang-installer/src/downloader/pypi.rs:435-438
Timestamp: 2026-03-30T06:38:21.052Z
Learning: In `clang-installer/src/downloader/pypi.rs`, the PyPI downloader intentionally uses `info.digests.first()` with an `if let Some(digest)` guard (no fail-closed on missing digest). This is acceptable because the downloader exclusively targets the `clang-format` and `clang-tidy` PyPI packages, which reliably publish digests. Failing closed for a missing digest was explicitly declined by the maintainer (2bndy5).

Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-rs PR: 224
File: cpp-linter/src/clang_tools/clang_format.rs:103-139
Timestamp: 2025-12-11T20:19:13.754Z
Learning: In cpp-linter-rs, run_clang_format in cpp-linter/src/clang_tools/clang_format.rs must execute two clang-format commands sequentially when format_review is enabled: first to get formatted output, then to get XML replacements based on the original unformatted content. Concurrent execution would create a race condition because the commands operate on different file states. This sequential pattern is intentional and necessary given clang-format's capabilities.

Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-rs PR: 54
File: cpp-linter/src/clang_tools/mod.rs:149-159
Timestamp: 2024-11-23T14:10:31.760Z
Learning: Clang tools' `--version` output is consistent across all released versions, so adding extra error handling or unit tests for version extraction may be unnecessary. Tests are run after the clang tools are installed.

Learnt from: 2bndy5
Repo: cpp-linter/cpp-linter-rs PR: 279
File: clang-installer/src/version.rs:301-304
Timestamp: 2026-03-30T06:22:32.553Z
Learning: In this repo’s Rust tests (run via `cargo-nextest` configured by `.config/nextest.toml`), each test runs in its own process. Therefore, `unsafe { std::env::set_var(...) }` used inside tests is safe with respect to cross-test environment variable pollution, and you generally do not need to implement a save-and-restore guard for env vars in test code.

Comment on lines +40 to +45
/// ```
/// use clang_installer::ProgressBar;
/// let mut progress_bar = ProgressBar::new(Some(100), "Downloading");
/// progress_bar.render().unwrap(); // render 0% state
/// progress_bar.inc(50).unwrap(); // render 50% state
/// progress_bar.inc(50).unwrap(); // render 100% state
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

cat -n clang-installer/src/progress_bar.rs | head -100

Repository: cpp-linter/cpp-linter-rs

Length of output: 5007


Fix the example to use Option<NonZero<u64>>.

ProgressBar::new expects Option<NonZero<u64>>, but the doctest passes Some(100) which is Option<u64>. This type mismatch will prevent the doctest from compiling.

Suggested fix
 /// ```
 /// use clang_installer::ProgressBar;
-/// let mut progress_bar = ProgressBar::new(Some(100), "Downloading");
+/// use std::num::NonZero;
+/// let mut progress_bar = ProgressBar::new(Some(NonZero::new(100).unwrap()), "Downloading");
 /// progress_bar.render().unwrap(); // render 0% state
 /// progress_bar.inc(50).unwrap(); // render 50% state
 /// progress_bar.inc(50).unwrap(); // render 100% state
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
/// ```
/// use clang_installer::ProgressBar;
/// let mut progress_bar = ProgressBar::new(Some(100), "Downloading");
/// progress_bar.render().unwrap(); // render 0% state
/// progress_bar.inc(50).unwrap(); // render 50% state
/// progress_bar.inc(50).unwrap(); // render 100% state
///
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@clang-installer/src/progress_bar.rs` around lines 40 - 45, The doctest passes
an Option<u64> to ProgressBar::new but the signature expects
Option<NonZero<u64>>; update the example to construct a NonZero<u64> and pass it
as Some(NonZero::new(...).unwrap()). Specifically modify the example around
ProgressBar::new to import or reference std::num::NonZero and call
ProgressBar::new(Some(NonZero::new(100).unwrap()), "Downloading") so the doctest
compiles with the correct type for ProgressBar::new.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dependencies Pull requests that update a dependency file enhancement New feature or request rust Pull requests that update Rust code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add new mechanism for installing clang tools

1 participant